-
-
Notifications
You must be signed in to change notification settings - Fork 452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(lint): options for noLabelWithoutControl
are optional
#4138
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #4138 will degrade performances by 6.34%Comparing Summary
Benchmarks breakdown
|
@@ -159,14 +152,124 @@ impl Rule for NoLabelWithoutControl { | |||
|
|||
#[derive(Clone, Debug, Default, Deserialize, Deserializable, Eq, PartialEq, Serialize)] | |||
#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | |||
#[serde(rename_all = "camelCase", deny_unknown_fields)] | |||
#[serde(rename_all = "camelCase", deny_unknown_fields, default)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the serde(default)
attribute should be enough to make optional the fields.
Is there an interest to have both Option<>
and serde(default)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it might make more sense to use plain Vec
s only. It uses less memory, and makes consuming code easier. The only exception should be when an empty vector and None
are semantically different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try the serde(default)
, but the issue is the schema, not the default values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i am not mistaken, schemars recognizes serde(default)
@@ -161,16 +161,16 @@ We would like to set the options in the `biome.json` configuration file: | |||
} | |||
``` | |||
|
|||
The first step is to create the Rust data representation of the rule's options. | |||
The first step is to create the Rust data representation of the rule's options. Each option must be wrapped in a `Option`, this is required so the configuration schema won't mark them as required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then I see this as motivation for the Option
wrappers :) Could we make the configuration schema respect the default
annotations in addition to only Option
?
Co-authored-by: Arend van Beelen jr. <[email protected]>
Summary
Closes #4125
I updated the contribution guide too. I took the opportunity to do some small refactor, and move functions inside the options struct, so we don't pass around the same object.
Test Plan
CI should pass. I checked the code generated files manually.